Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(platform): smart filter bar #7504

Merged
merged 9 commits into from
Jan 13, 2022
Merged

feat(platform): smart filter bar #7504

merged 9 commits into from
Jan 13, 2022

Conversation

N1XUS
Copy link
Contributor

@N1XUS N1XUS commented Jan 4, 2022 •
edited
Loading

Related Issue(s)

closes #5291 closes #7442

Description

Created new 'Smart Filter Bar' component. -> https://deploy-preview-7504--fundamental-ngx.netlify.app/#/platform/smart-filter-bar
Refactored Platform Table data source and data provider classes.
Adjusted Platform form container component to support inline layout.
Refactored Platform table resize column service.
Adjusted Platform select component to support filling full width.
Adjusted Platform date picker to support filling full width.
Added new component to form generator - multi-input.

Screenshots

image
image
image

Please check whether the PR fulfills the following requirements

During Implementation
  1. Visual Testing:
  • visual misalignments/updates
  • check Light/Dark/HCB/HCW themes
  • RTL/LTR - proper rendering and labeling
  • responsiveness(resize)
  • Content Density (Cozy/Compact/(Condensed))
  • States - hover/disabled/focused/active/on click/selected/selected hover/press state
  • Interaction/Animation - open/close, expand/collapse, add/remove, check/uncheck
  • Mouse vs. Keyboard support
  • Text Truncation
  1. API and functional correctness
  • check for console logs (warnings, errors)
  • API boundary values
  • different combinations of components - free style
  • change the API values during testing
  1. Documentation and Example validations
  • missing API documentation or it is not understandable
  • poor examples
  • Stackblitz works for all examples
  1. Accessibility testing
  2. Browser Testing - Edge, Safari, Chrome, Firefox
PR Quality

@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from c9d6068 to dbac5a0 Compare January 4, 2022 12:19
@N1XUS N1XUS requested review from a team and removed request for valorkin and fkolar January 4, 2022 12:19
@netlify
Copy link

netlify bot commented Jan 4, 2022

✔️ Deploy Preview for fundamental-ngx ready!

🔨 Explore the source changes: c9d6068

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-ngx/deploys/61d43b230ffd1000074ce143

😎 Browse the preview: https://deploy-preview-7504--fundamental-ngx.netlify.app

@netlify
Copy link

netlify bot commented Jan 4, 2022 •
edited
Loading

✔️ Deploy Preview for fundamental-ngx ready!

🔨 Explore the source changes: 4ccdb86

🔍 Inspect the deploy log: https://app.netlify.com/sites/fundamental-ngx/deploys/61de9c101c065000075c6957

😎 Browse the preview: https://deploy-preview-7504--fundamental-ngx.netlify.app

@SAP SAP deleted a comment from lgtm-com bot Jan 4, 2022
@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch 6 times, most recently from 1e83c4e to 06d9b6e Compare January 5, 2022 09:09
@SAP SAP deleted a comment from lgtm-com bot Jan 5, 2022
@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from 06d9b6e to 89cd428 Compare January 5, 2022 09:39
dmitry-stepanenko
dmitry-stepanenko reviewed Jan 5, 2022
View reviewed changes
Copy link
Contributor

@dmitry-stepanenko dmitry-stepanenko left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big one! Unfortunately wasn't able to play around with smart filter component due to errors while using filters

angular.json Outdated Show resolved Hide resolved
apps/docs/src/app/platform/component-docs/platform-table/platform-table.module.ts Outdated Show resolved Hide resolved
.../components/smart-filter-bar-condition-field/smart-filter-bar-condition-field.component.html Show resolved Hide resolved
...ar/components/smart-filter-bar-settings-dialog/smart-filter-bar-settings-dialog.component.ts Outdated Show resolved Hide resolved
libs/platform/src/lib/smart-filter-bar/directives/smart-filter-bar-subject.directive.ts Outdated Show resolved Hide resolved
libs/platform/src/lib/smart-filter-bar/package.json Outdated Show resolved Hide resolved
libs/platform/src/lib/smart-filter-bar/tsconfig.json Show resolved Hide resolved
libs/platform/src/lib/table/domain/table-data-provider.ts Outdated Show resolved Hide resolved
@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch 6 times, most recently from 4a101f8 to ec3d6da Compare January 6, 2022 11:39
@SAP SAP deleted a comment from lgtm-com bot Jan 6, 2022
@dmitry-stepanenko
Copy link
Contributor

  1. There's no way to clear search if I want to see all results again
    image
    Also it's slightly confusing all filters are required. Maybe it would be better to have only one of them required in examples?

2.It incorrectly process absence of value

Jan-06-2022.13-30-06.mp4
  1. There's an error when trying to remove value
Jan-06-2022.13-32-31.mp4
  1. Filters count is invalid. Even if I remove one of the values, it will still increment the count
Jan-06-2022.13-35-48.mp4

@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from ec3d6da to 21d45e0 Compare January 6, 2022 12:35
@N1XUS N1XUS requested a review from platon-rov January 10, 2022 15:56
@InnaAtanasova
Copy link
Contributor

Great job!
I think the Reset is not working the way it's expected:
Screen Shot 2022-01-10 at 12 24 14 PM

Is it possible to add an example with Dynamic Page or it's not in the scope of this PR?

@N1XUS
Copy link
Contributor Author

N1XUS commented Jan 10, 2022 •
edited
Loading

Great job! I think the Reset is not working the way it's expected: Screen Shot 2022-01-10 at 12 24 14 PM

Is it possible to add an example with Dynamic Page or it's not in the scope of this PR?

It wasn't planned in this iteration, but I can try.
If you make changes to the filters selection in this dialog, and click on Reset button, it will reset to initial state. Since some of the fields are required and visible by default, they will be automatically selected.

@github-actions github-actions bot removed the stale label Jan 11, 2022
@N1XUS
Copy link
Contributor Author

N1XUS commented Jan 11, 2022

@InnaAtanasova example with dynamic page has been added.

@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from 0954e13 to 88f01a8 Compare January 11, 2022 12:39
platon-rov
platon-rov reviewed Jan 11, 2022
View reviewed changes
Copy link
Contributor

@platon-rov platon-rov left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good, thank you!

Found that example in dynamic page doesn't scroll properly.

Screen.Recording.2022-01-11.at.15.24.55.mov

@N1XUS
Copy link
Contributor Author

N1XUS commented Jan 11, 2022

Good, thank you!

Found that example in dynamic page doesn't scroll properly.

Screen.Recording.2022-01-11.at.15.24.55.mov

It's more of the Dynamic Page issue itself. If I add more items to the table to increase the height of the dynamic page content, this behaviour won't be present

@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from 8b0e7eb to 97787da Compare January 11, 2022 14:08
@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from 97787da to 390c7f8 Compare January 11, 2022 14:11
@mikerodonnell89
Copy link
Member

Styling - the additional padding provided by the form group causes the "name" input's left border to not left-align search input's left border, looks a bit strange:
Screen Shot 2022-01-11 at 9 50 35 AM

Search input's width changes when adding a character (and the X to clear the field appears) - should stay the same width

@fkolar
Copy link
Member

fkolar commented Jan 11, 2022

Do we have a design for this ? I am not even sure why we should call this smart filter bar, in the platform should be everything smart by default, to make the life of the app developer easier.

@InnaAtanasova
Copy link
Contributor

Do we have a design for this ? I am not even sure why we should call this smart filter bar, in the platform should be everything smart by default, to make the life of the app developer easier.

There's no design as it's a composition of components. In UI5 it's called Smart Filer Bar. I can send you a link to it in an email, can share it here :)

@fkolar
Copy link
Member

fkolar commented Jan 11, 2022

I believe there should be a design even for composition of the components. we have many examples in the platform like that. Cuz at the end the component is used certain why by app developer or by other component and it needs to feel right. IT does not matter what is inside. ;-(

@N1XUS N1XUS force-pushed the feat/5291-smart-filter branch from 00d3eef to 4ccdb86 Compare January 12, 2022 09:14
@N1XUS N1XUS merged commit 38a9410 into main Jan 13, 2022
@N1XUS N1XUS deleted the feat/5291-smart-filter branch January 13, 2022 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@InnaAtanasova InnaAtanasova InnaAtanasova left review comments

@dmitry-stepanenko dmitry-stepanenko dmitry-stepanenko approved these changes

@platon-rov platon-rov platon-rov approved these changes

@droshev droshev Awaiting requested review from droshev droshev is a code owner

Assignees

@N1XUS N1XUS

Labels
enhancement New feature or request
Projects
None yet
Milestone
Sprint 79 - Perth
Development

Successfully merging this pull request may close these issues.

bug(platform): select component uses label as a value SmartFilterBar Component
6 participants
@N1XUS @dmitry-stepanenko @InnaAtanasova @mikerodonnell89 @fkolar @platon-rov